Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle config errors #470

Closed
wants to merge 11 commits into from
Closed

Conversation

andrii-i
Copy link
Collaborator

@andrii-i andrii-i commented Nov 15, 2023

@andrii-i andrii-i added the enhancement New feature or request label Nov 15, 2023
@andrii-i andrii-i changed the title Handle errors Handle config errors Nov 16, 2023
@dlqqq
Copy link
Member

dlqqq commented Dec 8, 2023

@andrii-i

  1. What is the remaining work that needs to be done? Let's document this here so we have clear guidelines on when this can be merged.

  2. Can you include more screenshots in your PR description?

  3. If the chat backend fails, we should have some way of delivering the exception trace to the client. Can you include a screenshot showing the current UI for this?

@andrii-i
Copy link
Collaborator Author

andrii-i commented Dec 8, 2023

@dlqqq Thank you for looking into this.

  1. Current state of this PR / what I planned to implement initially: whatever config errors are present, initialize config manager with errors, save errors as instances of ConfigErrorModel, pass them to the front end as a part of serialized DescribeConfigResponse, reset problematic values to default accordingly to schema, show errors and/or warnings to the user on the welcome screen as MUI alert. I discussed this approach with @3coins and he made some good arguments that I agree with: users should not be able to edit config directly so there is no use in gracefully handling config errors other than lm or em id errors. We should allow config manager to fail unless it's lm or em id (already done). Instead it would be better to change how API AuthenticationError is handled:

I'm working on this now, looks much easier to implement that what I tried earlier. Will probably open a new PR as there are a lot of changes in this PR that won't be needed, will be able to update on the progress later today. Within this PR, I have refactored large ConfigManager._init_config function into sub-functions, that would also be good to merge.

Would also love to hear your thoughts and/or suggestions.

  1. See (3).
  2. Below is a screenshots of how exception trace is shown to the user in "current state" of this PR. Please note that I'm pivoting away from this. See below.
image
  • Here is how API Authentication Error currently shown to the user:
image
  • Here is a mockup of what I'm currently working on to achieve:
image

@andrii-i andrii-i closed this Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve config errors handling
2 participants